Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML] Anomaly Detection: add anomalies map to explorer for jobs with 'lat_long' function #88416

Merged
merged 15 commits into from
Jan 29, 2021

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Jan 14, 2021

Summary

  • Adds Maps embeddable map to anomaly explorer
  • Map is shown for jobs with 'lat_long' function when anomalies are filtered by swimcell selection
  • Map content if filtered by severity when severity control is updated
  • A map with the top anomaly is shown for up to 6 partition fields

image

Different types of jobs selected:

image

When maps or embeddable plugin are not enabled we show a message to the user and only show charts that can be shown:

image

NOTES

  • Custom styling for this type of layer should be supported by maps in the near future - this will allow us to match color of the points on the map to the severity colors.

Checklist

Delete any items that are not applicable to this PR.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@peteharverson
Copy link
Contributor

Love the embedded map in the Anomaly Explorer!

Some thoughts:

  • did you try using multiple maps, as we have multiple charts for metric or population anomalies? So you'd see one map for each partition field value with one pair of actual / typical points. Not sure how performant multiple maps would be?
  • As you noted in the description above, will be nice when we can match the color of the anomaly dot to the severity.

@walterra
Copy link
Contributor

This looks great! Agree with Pete's thought: What if we make this "just" another chart type of the charts when you drill down and click on anomalies (e.g. regular line chart, rare chart, population chart, map). For those charts we don't show typical values though just actuals, so maybe actual could be an optional layer but disabled by default to keep the data on display by default consistent with other charts?


return (
<div
className={'mlFieldDataCard__geoContent'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this className here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed 👍

@alvarezmelissa87 alvarezmelissa87 changed the title [ML] Anomaly Detection: add anomalies map to explorer for jobs with 'lat_long' function WIP[ML] Anomaly Detection: add anomalies map to explorer for jobs with 'lat_long' function Jan 15, 2021
@alvarezmelissa87 alvarezmelissa87 changed the title WIP[ML] Anomaly Detection: add anomalies map to explorer for jobs with 'lat_long' function [ML] Anomaly Detection: add anomalies map to explorer for jobs with 'lat_long' function Jan 25, 2021
@alvarezmelissa87
Copy link
Contributor Author

This has been updated and is ready for another look when you get a chance. cc @walterra, @peteharverson 🙏

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great progress! Added some more comments.

// import { AnomalyRecordDoc } from '../../../../common/types/seriesConfig.chartData';

interface Props {
seriesConfig?: any; // object;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since all other charts code is still plain JS, agree to use the any here in this PR. I added an item to this charts meta-issue that we should migrate to TS #21163.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in a9b5c32580d12c8a582b382049631e9709a7230a

Comment on lines 40 to 46
if (!embeddablePlugin) {
throw new Error('Embeddable start plugin not found');
}
if (!mapsPlugin) {
throw new Error('Maps start plugin not found');
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, for the file/data visualizer the maps plugin is defined as an optional plugin. Are there plans for a more graceful fallback in the UI if the plugin is not available?

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct - will update to optional as it should be and show a toast with a message that the plugin is disabled but let the anomaly explorer load normally otherwise 👍

throw new Error('Maps start plugin not found');
}

const factory: any = embeddablePlugin.getEmbeddableFactory(MAP_SAVED_OBJECT_TYPE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like embeddablePlugin is fully typed, can we get rid of the any?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in a9b5c32580d12c8a582b382049631e9709a7230a

const factory: any = embeddablePlugin.getEmbeddableFactory(MAP_SAVED_OBJECT_TYPE);

const input: MapEmbeddableInput = {
id: uuid.v4(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on other EUI examples, we use EUI's htmlIdGenerator is other places like this:

import { htmlIdGenerator } from '@elastic/eui';
const id = useMemo(() => htmlIdGenerator()(), []);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 478a75725eb997b45a1c62becae5c72bb4d9a1c2

if (!factory) {
throw new Error('Map embeddable not found.');
}
const embeddableObject: any = await factory.create({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to get rid of that any.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 478a75725eb997b45a1c62becae5c72bb4d9a1c2

>
<div
data-test-subj="xpack.ml.explorer.embeddedMapPanel"
className="embPanel__content"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this class necessary? I don't see any other reference to it in the ML plugin anywhere else.

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 478a75725eb997b45a1c62becae5c72bb4d9a1c2

Comment on lines 144 to 151
style={{
width: '100%',
height: '100%',
display: 'flex',
flex: '1 1 100%',
zIndex: 1,
minHeight: 0, // Absolute must for Firefox to scroll contents
}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this to a custom class in an SCSS file?

.catch((error) => {
console.error(error);
});
if (!isGeoMap) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hit an error when I try to view different types of jobs (e.g. high-count and geomap), a click in the overall swimlane results in this error:

image

Since this file takes care of getting data for all charts it needs to consider the possibility of different types of charts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed by 478a75725eb997b45a1c62becae5c72bb4d9a1c2

@peteharverson
Copy link
Contributor

peteharverson commented Jan 26, 2021

Looks like there is an issue with display of the detector function in the chart info tooltip for lat_long detectors. It says null where it should be lat_long:

image


Thanks for catching that, @peteharverson - fixed in 478a75725eb997b45a1c62becae5c72bb4d9a1c2

)}
</MlTooltipComponent>
);
if (chartType === CHART_TYPE.SINGLE_METRIC) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably one for a follow up, but would be nice if the SINGLE_METRIC and DISTRIBUTION charts expanded to the height of the map in this situation (preferable than shrinking the map height to match those chart heights I think).

image

@qn895
Copy link
Member

qn895 commented Jan 28, 2021

LGTM 🎉

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
ml 1728 1730 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 6.5MB 6.5MB +5.7KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested latest changes, and LGTM

@alvarezmelissa87 alvarezmelissa87 merged commit a08895d into elastic:master Jan 29, 2021
@alvarezmelissa87 alvarezmelissa87 deleted the ml-add-explorer-maps branch January 29, 2021 15:44
alvarezmelissa87 added a commit that referenced this pull request Jan 29, 2021
…lat_long' function (#88416) (#89738)

* wip: create embedded map component for explorer

* add embeddedMap component to explorer

* use geo_results

* remove charts callout when map is shown

* add translation, round geo coordinates

* create GEO_MAP chart type and move embedded map to charts area

* remove embedded map that is no longer used

* fix type and fail silently if plugin not available

* fix multiple type of jobs charts view

* fix tooltip function and remove single viewer link for latlong

* ensure diff types of jobs show correct charts. fix jest test

* show errorCallout if maps not enabled and is lat_long job

* use shared MlEmbeddedMapComponent in explorer

* ensure latLong jobs not viewable in single metric viewer

* update jest test
@lcawl lcawl added the release_note:feature Makes this part of the condensed release notes label Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants